Skip to content

fix(connectors): the external feed sync reads membership before it writes (TASK-164) - #1952

Merged
lilyshen0722 merged 1 commit into
mainfrom
kai/task164-external-feed-membership
Sep 27, 2026
Merged

lilyshen0722 merged 1 commit into
mainfrom
kai/task164-external-feed-membership

Conversation

@lilyshen0722

Copy link
Copy Markdown
Contributor

Closes the exception #1940's body names: every other connector site reads the strict predicate, and this one read nothing.

The gap

syncExternalFeeds loaded integrations and went straight to the provider. No membership read anywhere in the service, so it could not tell a creator from an ordinary departed member:

  • the flag-on path (EXTERNAL_FEED_PERSIST_POSTS=1) wrote posts as integration.createdBy;
  • the default path (appendIntegrationBuffer + enqueueCuratorEvents) handed that owner's feed to the pod's curator agents.

createMessage 403s that same owner. The owner can stop being a member without anyone touching the integration — leavePod filters them out of members, agent cleanup $pulls them — and the row kept syncing as them. Found by Wren (74669) while closing TASK-161.

The fix

The read goes before the provider call, at the top of the per-integration body: isListedPodMember(pod, integration.createdBy), the same predicate the pod's own write paths implement. A departed owner therefore makes no provider request, advances no cursor, and writes nothing on either path — buffering and curator events are both behind it.

A pod that is gone pauses too. It cannot name a member, and the alternative (sync into nothing, report success) is the failure mode the row is about. That one is my addition beyond the row's text, flagged here rather than buried.

Pause shape, per Wren: status: 'error' with a Commonly-written errorMessage and errorMessageUserFacing: true, written as its own update rather than thrown into the catch — the catch stamps errorMessageUserFacing: false, which is right for provider text and wrong for copy a person must read. isActive stays true so the row stays on the owner's Connectors page; that page has no action for x/instagram (integrations.ts:681), which is why the copy carries the next step itself. 'error' also takes the row out of the sync query (status: 'connected'), so the pause is written once per connection rather than every tick, and a re-saved row pauses again on the next sync. No resume logic is added.

Admin rows would have paused at birth, which is why this PR also touches the admin route: only the first requester of the Global Social Feed pod became a Mongo member (at pod creation), and a second admin configuring the other feed type was mirrored into PG alone (ensureGlobalPodPostgresSync). ensureGlobalSocialFeedPod now $addToSets the requester into Mongo members — the surface the predicate reads and the pod's own write paths enforce — and hands on the refetched pod. createdBy is untouched: it is the row's owner, not a membership record.

Consumers: schedulerService (two call sites) and the admin /sync route. Both get the same behaviour; the paused entry comes back as paused: true, success: false with the reason as content.

Witnesses (5 new arms, 23 tests in the two suites)

arm asserts
departed owner registry.get not called, Post.insertMany not called, no curator event, no $push to the buffer, one write: status: 'error' + errorMessageUserFacing: true + the reason, isActive untouched, result paused: true
departed owner, flag on Post.find not called, Post.insertMany not called, no provider call
pod gone no provider call, paused
listed owner (control) provider called, buffer pushed, curator events as today, paused undefined
admin: second admin $addToSet written, pod refetched, the new row's createdBy is that admin
admin: already listed (control) no updateOne, no refetch

Ledger — 10 mutations, each alone, 9 no-survivor

mutation red
M1 no membership read at all (the pre-TASK-164 shape) 3 — departed, flag-on, pod-gone
M3 pause copy marked diagnostic, not user-facing 1 — departed
M4 pause keeps the row in the sync query (no status) 1 — departed
M5 pause also deactivates the row (drops it off the page) 1 — departed
M6 gate moved after the provider call 3 — departed (spends the call), flag-on, pod-gone
M7 gate checks the wrong principal (integration._id) 5 — every listed-owner arm
M10 raw members.includes(owner) instead of the shared predicate 5 — every listed-owner arm
M8 admin route stops listing a second admin 1 — the second-admin arm
M9 admin route adds unconditionally 1 — the already-listed control
M2 !pod clause dropped none — expected survivor, disclosed

M2 is a tripwire, not coverage. isListedPodMember already refuses a null pod, so behaviour cannot distinguish the two forms. I kept the clause because relying on a util's nil handling for a pod lookup couples this branch to that implementation detail, and the pod-gone arm reads better for it.

M7 and M10 both redden the controls rather than the departure arms, and that shape is the point. A wrong principal, or reference equality instead of value comparison, pauses everyone — so the arms that can see it are the ones asserting a listed owner is not paused. The departure arms pass under both for the wrong reason, and saying so is more useful than a count.

M10 is why the fixture changed. It survived the first ledger because my pod stub and my integration fixture shared one ObjectId instance, so includes passed by reference. mockPodMembers now stores a fresh instance carrying the same hex, which is what a lean read produces — the arm would otherwise have certified a fixture rather than the code. This is the TESTING.md fixture-shape rule one layer over: the mutation did not fail to apply, it applied and was admitted by the instrument.

Verification

  • __tests__/unit/services + __tests__/unit/routes — 300 suites / 2817 tests green.
  • Lint: services/externalFeedService.ts and routes/admin/globalIntegrations.ts 0 errors, 0 on touched lines. The two test files are at their main baseline (16 and 12 import/* errors, the repo-wide pattern for .js tests requiring .ts modules, not gated); the new Pod require carries that same pair. Two object-curly-newline violations I introduced by copying the surrounding one-line body: {...} style are fixed — main has one such pre-existing violation at admin.globalIntegrations.test.js:243, untouched.

…ites (TASK-164)

`syncExternalFeeds` read no membership at all. The flag-on path wrote posts AS
`integration.createdBy`, and the default path handed the owner's feed to the
pod's curator agents; `createMessage` would 403 that same owner. Found by Wren
(74669) while closing TASK-161, and kept out of #1940 because it needs a read
plus a pause rule rather than a predicate swap.

The read goes at the top of the per-integration body, before `syncRecent`, so a
departed owner makes no provider call and advances no cursor either. The
predicate is `isListedPodMember` - the same rule the pod's own write paths
implement - so the feed is never more permissive than the pod it writes into.
A pod that is gone pauses too: there is no surface left to write into, and
syncing into nothing looks like a healthy run.

Pause shape (wren, TASK-164): `status: 'error'` with a Commonly-written
`errorMessage` and `errorMessageUserFacing: true`, written as its own update
rather than thrown into the catch, which stamps the flag false. `isActive` stays
true, so the row stays on the owner's Connectors page - that page has no action
for x/instagram, which is why the copy carries the next step itself. `'error'`
also takes the row out of the sync query (`status: 'connected'`), so the pause is
written once per connection rather than every tick; a re-saved row pauses again
on the next sync, and no resume logic is added.

Also, admin rows would have paused at birth: only the FIRST requester of the
Global Social Feed pod became a Mongo member (at creation), and a second admin
configuring the other feed type was mirrored into PG alone. `ensureGlobalSocialFeedPod`
now `$addToSet`s the requester into Mongo `members` and hands on the refetched
pod.

Not in scope, noted by wren: those global routes find their pod by name, and pod
names are not unique.
@lilyshen0722
lilyshen0722 added this pull request to the merge queue Sep 27, 2026
Merged via the queue into main with commit e748ac3 Sep 27, 2026
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant